Storing system descriptor #265
Conversation
sdjukicTT
requested review from
ajakovljevicTT,
kmitrovicTT and
mrakitaTT
as code owners
|
||||||||||||||
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #265 +/- ##
==========================================
+ Coverage 78.06% 78.15% +0.09%
==========================================
Files 21 21
Lines 1044 989 -55
==========================================
- Hits 815 773 -42
+ Misses 229 216 -13 ☔ View full report in Codecov by Sentry. |
src/common/module_builder.cc
Outdated
| @@ -197,6 +197,7 @@ void ModuleBuilder::convertFromTTIRToTTNN( | |||
| mlir::PassManager ttir_to_ttnn_pm(mlir_module.get()->getName()); | |||
|
|
|||
| mlir::tt::ttnn::TTIRToTTNNBackendPipelineOptions options; | |||
| options.systemDescPath = "system_desc.ttsys"; | |||
There was a problem hiding this comment.
Can we extract this path into client_instance or module_builder static attribute? Also, it might be nice to have a folder instead of just storing it in root. That way, we could even delete the system_desc when ClientInstance.destroy() is called, leaving less things in memory.
There was a problem hiding this comment.
I think we should be very of keeping thing in root, and this adds another opportunity to be more verbose (for example, by calling the folder "system_descriptors"). However, I might be wrong. @mrakitaTT do you have an opinion on this?
There was a problem hiding this comment.
Touched on this topic in my other comment
sdjukicTT
force-pushed
the
sdjukic/store-system-desc
branch
from
90d1d7f to
e1cd701
Compare
| @@ -40,6 +40,8 @@ class ModuleBuilder { | |||
| // code. Currently hardcoded to one, as we only support one-chip execution. | |||
| size_t getNumAddressableDevices() const { return 1; } | |||
|
|
|||
| static constexpr std::string_view system_desc_path = "system_desc.ttsys"; | |||
There was a problem hiding this comment.
Any particular reason why this is not just a plain string?
There was a problem hiding this comment.
Yes this can be a plain string, there is no benefit to keep it as string_view. See more info here regarding string vs string_view (std string_view is implementing the same thing as abseil's).
There was a problem hiding this comment.
Ideally, we would store tt::runtime::SystemDesc object inside the ClientInstance and pass it through the Compile function to the ModuleBuilder which would then pass it to the createTTIRToTTNNBackendPipeline. Since tt-mlir currently only supports passing a path to the descriptor and is not easy to change to support passing already parsed object, as a temporary solution we need to save it on the disk as pass the path to the compiler.
I propose these changes:
- Create a member variable
tt::runtime::SystemDesc m_system_descriptorinClientInstancewhich will be set in theClientInstance::PopulateDevices()function from thesystem_descvariable. - Create a member variable
std::string m_cached_system_descriptor_pathwhich should be set inside theClientInstanceconstructor to the combination ofstd::filesystem::temp_directory_path()directory and some file name that should be unique from other programs, for examplett_pjrt_system_descriptorplus maybe name of the device architecture, and maybe even some client id if there is some in pjrt structures. - In the
ClientInstance::PopulateDevices()initializem_system_descriptorwithsystem_desc, and then store it intom_cached_system_descriptor_pathwith a TODO comment to remove that once the support in tt-mlir is done to pass the system descriptor object. Check ifstoremethod checks for errors, if not we should check. - In the
ClientInstance::Compilefunction pass them_cached_system_descriptor_pathto themodule_builder_->buildModulecall. - In the
ClientInstance::~ClientInstance()remove the cached system descriptor, as you already do.
| @@ -28,6 +28,7 @@ ClientInstance::ClientInstance(std::unique_ptr<Platform> platform) | |||
|
|
|||
| ClientInstance::~ClientInstance() { | |||
| DLOG_F(LOG_DEBUG, "ClientInstance::~ClientInstance"); | |||
| std::remove(ModuleBuilder::system_desc_path.data()); | |||
There was a problem hiding this comment.
Why is ClientInstance destructor doing this? Why even do this in the first place? I don't think I have ever seen a string being cleaned up.
There was a problem hiding this comment.
std::remove is cleaning up a file stored in that path
Ticket
#201
Problem description
Default system descriptor used in
TTIRToTTNNBackendPipelinecan't be used for multichip, actual current system descriptor is needed.What's changed
We store the system descriptor and then set
options.systemDescPath.Checklist